Skip to content

Fix issue with nil properties not being set and passing hash with string keys issue #164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ErickFabian merged 4 commits into from
May 11, 2021
Merged

Fix issue with nil properties not being set and passing hash with string keys issue #164

ErickFabian merged 4 commits into from
May 11, 2021

Conversation

@ErickFabian
Copy link
Contributor

@ErickFabian ErickFabian commented Apr 14, 2021 •
edited
Loading

Fixes:

  • Properties being set to nil would not be updated

javierg
javierg requested changes Apr 14, 2021
View reviewed changes
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are 2 issues, can you create 2 different PRs?

@ErickFabian
Copy link
Contributor Author

ErickFabian commented Apr 14, 2021

@javierg Done, this is the other PR #165

@ErickFabian ErickFabian changed the base branch from 3.0.3 to 3.1 May 10, 2021 18:07
@javierg
Copy link
Member

javierg commented May 10, 2021

@ErickFabian can you write a test please. I am not sure if this change will make any missing attribute on the model being removed.

@ErickFabian
Copy link
Contributor Author

ErickFabian commented May 10, 2021

@javierg I've added a spec for setting nil to a property on a doc

@ErickFabian ErickFabian requested a review from javierg May 10, 2021 18:38
javierg
javierg reviewed May 11, 2021
View reviewed changes
test/property_manager_test.rb Outdated

class PropertyManagerTest < Test::Unit::TestCase
test 'write_attribute with nil value' do
doc = TestDoc.new(name: 'name')
Copy link
Member

@javierg javierg May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add more properties, other than the one being updated

@ErickFabian ErickFabian requested a review from javierg May 11, 2021 14:16
javierg
javierg approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ErickFabian ErickFabian merged commit ecccc4c into 3.1 May 11, 2021
@ErickFabian ErickFabian deleted the fix_update_properties branch May 11, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javierg javierg javierg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ErickFabian @javierg